Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

synocli-file: add ripgrep, initial rust support #3883

Merged
merged 18 commits into from
Jun 28, 2021

Conversation

Hylen
Copy link
Contributor

@Hylen Hylen commented Feb 14, 2020

This also adds support for building rust packages.

This solves issue #3799 (closes #3799)

I followed the Developers-HOW-TO and tested this using:
make arch-x64-6.1 arch-aarch64-6.1 arch-armada38x-6.1
I only tested installing arch-armada38x-6.1 as that's the device I own.

Tasks done:

  • create mk/spksrc.rust.mk top build rust projects
  • define RUST_TRIPLE based on ARCH
  • download rust targets on demand
  • define CARGO_HOME
  • validate all-supported targets (first rust build needs enhanced validation)
    • x64
    • armada370, armada38x
    • aarch64
    • hi3535
    • qoriq
    • evansport
    • ARMv5 (88f621)

Archs that are not supported (with make all-supported)

  • ppc853x

@hgy59
Copy link
Contributor

hgy59 commented Feb 14, 2020

@Hylen thanks for this great contribution!
I tested this and have some suggestions:

  • installing cargo to /spksrc/.cargo does not work, since you will mount the spksrc repository as volume to /spksrc and therefor the .cargo folder will not be visible inside the running docker container (I worked around this by installing to /opt/.cargo and adding /opt/.cargo/bin to the PATH)
  • when running the spksrc container as different user (not root!) the installed targets cannot be found and have to be installed with rustup target add again: We should add rust support into the framework to handle some generic tasks
  • I propose to set CARGO_HOME=/spksrc/distrib/cargo to use this folder as download cache as we do for downloaded packages (also as framework support)
  • I moved the definition of RUST_TRIPPLE in cross/ripgrep/Makefile after include ../../mk/spksrc.cross-cc.mk to fix make digests and make clean in this folder.
  • We prefer official releases over git-hash. Since 11.0.2 is an offical release we sould use this download method.

TODO moved to PR descritpion

I could push my suggestions, but prefer to investigate into the framework part first.

@hgy59
Copy link
Contributor

hgy59 commented Feb 14, 2020

Another TODO (done in separate PR #3884)

  • create generic ARCHES for armv7 (and maybe armv5) as we do for armv8 (aarch64) and x64.

@ymartin59
The same approach could be used for go packages and may be others, that use a more generic arch definition than some synology toolchains.

@Hylen
Copy link
Contributor Author

Hylen commented Feb 14, 2020

Hi @hgy59 ,

I should be thanking for this great framework!

I could push my suggestions, but prefer to investigate into the framework part first.

Do you want me to start clearing the TODO, or are you working on it? I could start working on downloading the official release etc. if you want me to do the framework stuff.

validat all-supported targets

What is this? I saw a comment about it when opening the pull request but couldn't find anything here:
https://github.com/SynoCommunity/spksrc/wiki/Developers-HOW-TO

@hgy59
Copy link
Contributor

hgy59 commented Feb 14, 2020

@Hylen With validation I mean that we have to validate, that the code created by rust for a specific RUST_TRIPLE creates compatible binaries for the specific synology ARCHS.
Particularly wheter the RUST_TRIPLE=armv5te-unknown-linux-gnueabi is compatible with arch-88f6281.

Inspired by your PR I am currently working on a generic ARM7 arch for modules built with go.
Go, rather than rust, is really synology toolchain independent, since the native go compiler is used to build the specific targets. With a generic ARM7 target we can omit to build separate targets for alpine armada370 armada375 armada38x armadaxp comcerto2k monaco.

I hope we will have the same benefit for targets built with rust, but rust depends on the toolchain specific compiler. But I am hopeful that we will get there.

The first todo is a very challenging task, so it will take some time.

For now I pushed my suggestions (ignore the GENERIC_ARCHS = ARM7 - it does nothing yet.

@hgy59
Copy link
Contributor

hgy59 commented Feb 16, 2020

As other grep tools I moved ripgrep to the synocli-file package.
And moved the standalone version to diyspk.

This PR is now ready to merge and publish updated synocli-file package.
The generic ARMv7 configuration (#3884) is not relevant for synocli-file that contains mostly non rust built packages.

ARMv7 packages of ripgrep will be available with diyspk/ripgrep when PR #3884 is merged.

@hgy59 hgy59 changed the title ripgrep: initial package v. 11.0.2 synocli-file: add ripgrep, initial rust support Feb 16, 2020
@hgy59 hgy59 added the update request to update existing package label Feb 16, 2020
@hgy59
Copy link
Contributor

hgy59 commented Feb 16, 2020

Initial packages for validation are available here: ripgrep_v11.0.2_pre

Please report working/non working results here.

@AdithyaBenny (as you originally requested ripgrep) what kind of arch is your diskstation? can you test it?

@Hylen
Copy link
Contributor Author

Hylen commented Feb 17, 2020

@hgy59 Where is your work in progress for this?

I see you checked off everything but the validation above, and I'm afraid I can't help with that except armada38x. So I guess I'll just wait for others to validate the packages?

In the meantime I'm interested in trying to get BitWarden (#3712) to** work too, so I would like to work on your latest patch.

@Hylen
Copy link
Contributor Author

Hylen commented Feb 17, 2020

Ah, I see you pushed it to my fork. Thanks!

@Hylen
Copy link
Contributor Author

Hylen commented Feb 17, 2020

Looked through your patches, looks great! Thanks for improving this.

What is diyspk? Stuff not published to the repository?

@hgy59
Copy link
Contributor

hgy59 commented Feb 17, 2020

yes diyspk is "do it yourself spk" for packages contained in synocli-, while only the synocli- are published.
It's also good for development, when you have a small package, instead of the larger synocli-* that need much more time to build.

@hgy59
Copy link
Contributor

hgy59 commented Feb 17, 2020

I tried bitwarden too, based on the rust infrastructure developed for this PR.
First I had to enhance the build to allow nightly rust builds instead of stable.
Second enhancement was to add additional arguments like --features sqlite.
Unfortunatly the build of bitwarden_rs fails just before the end of about 482 modules at migration_macros. I tried bitwarden_rs versions 1.13.1, 1.13.0, 1.12.0, 1.10.0 and all have the same error output.

= note: /spksrc/toolchains/syno-x64-6.1/work/x86_64-pc-linux-gnu/bin/../lib/gcc/x86_64-pc-linux-gnu/4.9.3/../../../../x86_64-pc-linux-gnu/bin/ld: /spksrc/cross/bitwarden-rs/work-x64-6.1/bitwarden_rs-1.10.0/target/release/deps/liblibsqlite3_sys-b5d9472dc57c9340.rlib(sqlite3.o): unrecognized relocation (0x2a) in section `.text.sqlite3_libversion'
        /spksrc/toolchains/syno-x64-6.1/work/x86_64-pc-linux-gnu/bin/../lib/gcc/x86_64-pc-linux-gnu/4.9.3/../../../../x86_64-pc-linux-gnu/bin/ld: final link failed: Bad value
        collect2: error: ld returned 1 exit status

It looks like a problem with sqlite, but I couldn't find any useful information in the net.
And I am not familiary with rust.

And the rust build of bitwarden_rs took a very long time (felt like 2 hours, but may have been less)

@Hylen
Copy link
Contributor Author

Hylen commented Feb 17, 2020

I started doing exactly what you described, installing nightly etc. Feels like a waste if we're both working on it, but I'm happy to take a look if you want. Perhaps you can push your work in progress somewhere?

There's also the option of using either postgress (saw you had some work in progress for a package) or mariadb for the database. I'm using mariadb for other stuff, so I was going to try that but I think we'll be able to get all of them working if we want.

@hgy59
Copy link
Contributor

hgy59 commented Feb 17, 2020

The rust makefile is worth to push here.
And attached you find my latest cross/bitwarden_rs/Makefile. (renamed as Makefile.txt to make upload possible)
Makefile

@Hylen
Copy link
Contributor Author

Hylen commented Feb 18, 2020

Ok, I merged our efforts and pushed it on the branch "bitwarden" in my fork. My build doesn't work for obvious reasons yet (I don't build all dependencies etc.) but we're a bit on the way I'd say. In the coming days I might find time to finalize.

(I didn't look at your makefile yet, still using my own)

@Hylen
Copy link
Contributor Author

Hylen commented Feb 20, 2020

I managed to cross compile bitwarden too btw. Pushed the progress to my fork.

@hgy59
Copy link
Contributor

hgy59 commented Feb 22, 2020

@Hylen thanks for bitwarden progress.

The cross/bitwarden_rs builds succeed, but the cross compiling in spk/bitwarden_rs builds fails (all archs except x64) with error: failed to run custom build command for 'ring v0.14.6'
Any idea why cross compiling in spk folder may fail?

@Hylen
Copy link
Contributor Author

Hylen commented Feb 26, 2020

That is really weird! I will take a look when I have some time over.

Copy link
Contributor

@ymartin59 ymartin59 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job.
Some minor improvements requests.
Are you sure all targets in mk/spksrc.cross-rust.mk are required?

Dockerfile Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@Hylen
Copy link
Contributor Author

Hylen commented Feb 28, 2020

@Hylen thanks for bitwarden progress.

The cross/bitwarden_rs builds succeed, but the cross compiling in spk/bitwarden_rs builds fails (all archs except x64) with error: failed to run custom build command for 'ring v0.14.6'
Any idea why cross compiling in spk folder may fail?

@hgy59 I took a look at this and it's related to the environment variable CC. Looks like CC gets set to the cross compiler, but rustup expects it to be the host compiler. I think this is what's causing it
depend_target: $(PRE_DEPEND_TARGET) @for depend in $(KERNEL_DEPEND) $(BUILD_DEPENDS) $(DEPENDS) ; \ do \ env $(ENV) $(MAKE) -C ../../$$depend ; \ done
where ENV probably already has been filled with the toolchain variables. I got the build to work by unexporting CC and CFLAGS (this made another host compilation fail because of a target include directory in the flags). I guess this isn't the solution we want in the end though, but I don't know how to fix it properly.

Pushed the progress to the bitwarden branch in my fork.

@Hylen
Copy link
Contributor Author

Hylen commented Feb 28, 2020

Great job.
Some minor improvements requests.
Are you sure all targets in mk/spksrc.cross-rust.mk are required?

@ymartin59 I gave it a quick look again now, and I can't find anything redundant by just inspecting the code. I don't completely understand the whole makefile system though, so I might be missing something. Perhaps @hgy59 knows this better?

@hgy59
Copy link
Contributor

hgy59 commented Feb 28, 2020

@ymartin59 the mk/spksrc.cross-rust.mk is based on mk/spksrc.cross-cc.mk and adjusted for rust.
I think most of the make rules are required. I tried to remove the kernel related stuff, but got some errors (can't remeber the details, it's some weeks ago), so I left the rest untouched. I too do not understand all dependencies (it's more than 30 years ago, that I professionally created makefiles).

@ymartin59
Copy link
Contributor

@hgy59 May you please rebase to take benefits of new Makefiles with "common" targets, so that to include them (instead of copy-paste)? Thank you in advance

@ymartin59 ymartin59 self-assigned this Apr 9, 2020
@hgy59 hgy59 added the build/rust Requires rust build system label Dec 30, 2020
mk/spksrc.cross-rust.mk Outdated Show resolved Hide resolved
mk/spksrc.cross-rust.mk Outdated Show resolved Hide resolved
@etcusrvar
Copy link
Contributor

I squashed, rebased, and fixed issues here: https://github.com/etcusrvar/spksrc/tree/hylen-rg-merge. With this, I was able to build and run rg on my syno device.

@publicarray
Copy link
Member

@hgy59 @Hylen @etcusrvar I've rebased the PR https://github.com/publicarray/spksrc/commits/ripgrep if you like I can force push it onto here.

Hylen and others added 12 commits June 26, 2021 20:10
This also adds support for building rust packages.
- install cargo to /opt/cargo
- download additional rust targets
- use officially released version of ripgrep
- make square icon
- add common Makefile for rust build and installation
- install rust targets on demand
- remove rust target installation from Dockerfile
- add ripgrep to synocli-file
- move spk/ripgrep to diyspk/ripgrep
- allow to define other release channel than 'stable'
- allow to define additional cargo build args
- adjust spksrc.cross-rust.mk
- adjust UNSUPPORTED_ARCHS
@hgy59 hgy59 merged commit bab9528 into SynoCommunity:master Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/rust Requires rust build system update request to update existing package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Package Request] ripgrep (rg)
5 participants